Fix destructor call in NotePlayHandleManager #3884

PhysSong · 2017-10-15T14:05:34Z

Fixes a regression from #3783 due to missing destructor call. It should have been added in 42e67d2, but not.
Since BufferManager::releaseBuffer() call is moved in #3783, buffer isn't freed(by MM_FREE()) in NotePlayHandle::done(). It isn't a memory leak because MemoryManager works as a garbage collector, but decreases usable memory.
This PR also makes sure PlayHandle::m_processingLock's destructor is called.

zonkmachine · 2017-10-15T14:46:35Z

I tested it with the 'football theme' from the original issue, #3223, and it works fine.

lukas-w · 2017-10-15T20:01:21Z

src/core/NotePlayHandle.cpp

@@ -160,6 +160,8 @@ void NotePlayHandle::done()
 	if( buffer() ) releaseBuffer();

 	unlock();
+
+	this->PlayHandle::~PlayHandle();


It should never be necessary to call destructors explicitly in C++. If PlayHandle's destructor is never called to release the buffer, something else is wrong here - if it is called, this patch will lead to the buffer being freed twice.

@lukas-w Usually yes, but I think no in this situation. Since NotePlayHandle uses memory allocated by NotePlayHandleManager and its destructor is never called, NotePlayHandle::done() should do what destructor does.
If you really don't like this, you may consider redesigning or removing NotePlayHandleManager.

Note: https://stackoverflow.com/questions/14187006/is-calling-destructor-manually-always-a-sign-of-bad-design

Then it's a bug in NotePlayHandleManager. The destructor should be called in NotePlayHandleManager::release because it's NotePlayHandleManager::acquire where placement-new is used. Also, it looks like NotePlayHandle::done()'s code actually belongs in ~NotePlayHandle.

Then renaming NotePlayHandle::done() to NotePlayHandle::~NotePlayHandle() and calling it in the NotePlayHandleManager::release would just be fine?

Edit: done by 2d07efd.

Yep, I think it's fine this way 👍 As far as I can see, done() is only called in NotePlayHandleManager::release() and simply plays the role of a destructor.

PhysSong · 2017-10-16T06:48:27Z

Now NotePlayHandle::release() will call parent classes' destructor correctly, always once. #1088 looks really bad for now...

PhysSong · 2017-10-16T07:18:49Z

Side note: NotePlayHandleManager seems not safe from ABA problem.
For example, s_available[ s_availableIndex.fetchAndAddOrdered( 1 ) + 1 ] = nph; isn't an atomic operation. It consists of two atomic operations: fetch-and-add s_availableIndex and store in s_available with the index. If a context switch occurs between them and some other operations are done, it may lead to wrong results.
Though it won't make a trouble frequently(since it is a rare case), this should be fixed later if it matters.

zonkmachine · 2017-10-17T09:17:14Z

Cool destructor. You can even hear it working... . ;)
(turn the volume down and loop it)
destructor.mmp.zip

lukas-w · 2017-10-17T09:35:18Z

@zonkmachine That file stops playing for me after a couple of seconds both with and without this PR merged.

Edit: Bisecting shows f23cf4e to be the commit that introduced this, so it's a regression from #3783

zonkmachine · 2017-10-17T09:55:12Z

I bisected wrongly. Loosing my favourite game. :(

lukas-w · 2017-10-17T12:08:39Z

Don't worry, you'll have enough chances (a.k.a. "bugs") to play this game again. :)

Fixes buffer noises when instruments don't write the whole buffer, such as bitinvader. Related: * #3884 (comment): #3884 (comment) * #3883 # #3383

lukas-w · 2017-10-17T18:25:27Z

@zonkmachine I did some debugging and the error seems to be within bitinvader not zeroing the buffer, so I added a line in PlayHandle do just that and guard against instruments that don't (#3883 comes to mind). Can you confirm 6fc4577 fixes it?

zonkmachine · 2017-10-17T19:44:11Z

It's definitely an improvement. There is no longer a thunderclap coming out of the reverb. I still get a NaN and the noise cuts out though when moving the tempo slider while looping.

I ran it with the fpe debug and got the backtrace below. It reminds me of a comment here which also had a problem file with a bitinvader in it.

Program received signal SIGFPE, Arithmetic exception.
[Switching to Thread 0x7fffdddde700 (LWP 19879)]
0x000000000052499f in InstrumentSoundShaping::processAudioBuffer (this=0x7fffeab2aca0, buffer=0x7fffeab2f5e0, frames=256, n=0x7fffeab218e8)
    at /home/zonkmachine/builds/lmms/lmms/src/core/InstrumentSoundShaping.cpp:206
206					float new_cut_val = EnvelopeAndLfoParameters::expKnobVal( cutBuffer[frame] ) *
(gdb) bt
#0  0x000000000052499f in InstrumentSoundShaping::processAudioBuffer (this=0x7fffeab2aca0, buffer=0x7fffeab2f5e0, frames=256, n=0x7fffeab218e8)
    at /home/zonkmachine/builds/lmms/lmms/src/core/InstrumentSoundShaping.cpp:206
#1  0x000000000062b3ae in InstrumentTrack::processAudioBuffer (this=0x7fffeab29720, buf=0x7fffeab2f5e0, frames=256, n=0x7fffeab218e8)
    at /home/zonkmachine/builds/lmms/lmms/src/tracks/InstrumentTrack.cpp:203
#2  0x00007fffdc3b3704 in bitInvader::playNote (this=0x7fffeab2c5a0, _n=0x7fffeab218e8, _working_buffer=0x7fffeab2f5e0)
    at /home/zonkmachine/builds/lmms/lmms/plugins/bit_invader/bit_invader.cpp:301
#3  0x000000000062c41e in InstrumentTrack::playNote (this=0x7fffeab29720, n=0x7fffeab218e8, workingBuffer=0x7fffeab2f5e0)
    at /home/zonkmachine/builds/lmms/lmms/src/tracks/InstrumentTrack.cpp:483

(gdb) bt full
#0  0x000000000052499f in InstrumentSoundShaping::processAudioBuffer (this=0x7fffeab2aca0, buffer=0x7fffeab2f5e0, frames=256, n=0x7fffeab218e8)
    at /home/zonkmachine/builds/lmms/lmms/src/core/InstrumentSoundShaping.cpp:206
        new_cut_val = 6025
        fcv = 25
        cutBuffer = 0x7fffddddd6c0
        resBuffer = 0x7fffddddd2b0
        old_filter_cut = 6025
        old_filter_res = 0
        frv = 0,790000021
        envTotalFrames = 32316
        envReleaseBegin = -370
#1  0x000000000062b3ae in InstrumentTrack::processAudioBuffer (this=0x7fffeab29720, buf=0x7fffeab2f5e0, frames=256, n=0x7fffeab218e8)
    at /home/zonkmachine/builds/lmms/lmms/src/tracks/InstrumentTrack.cpp:203
        offset = 0
        vol = 2,35106045e-38
        pan = 0 '\000'
        vv = {vol = {-1,07653206e+26, 4,59163468e-41}}
        DefaultVolumeRatio = 0,00999999978
#2  0x00007fffdc3b3704 in bitInvader::playNote (this=0x7fffeab2c5a0, _n=0x7fffeab218e8, _working_buffer=0x7fffeab2f5e0)
    at /home/zonkmachine/builds/lmms/lmms/plugins/bit_invader/bit_invader.cpp:301
        frames = 256
        offset = 0
        ps = 0x7fffeab2fde0

PhysSong · 2017-10-17T22:35:15Z

@zonkmachine It looks like #3775.

zonkmachine · 2017-10-17T22:39:49Z

Yes, that looks like it.

lukas-w · 2017-10-18T06:30:29Z

@zonkmachine So I guess it's not related to this PR or #3783?

zonkmachine · 2017-10-18T11:50:41Z

Yes, job done here.

Fix missing destructor call in NotePlayHandle

Fixes buffer noises when instruments don't write the whole buffer, such as bitinvader. Related: * LMMS#3884 (comment): LMMS#3884 (comment) * LMMS#3883 # LMMS#3383

lukas-w requested changes Oct 15, 2017

View reviewed changes

PhysSong force-pushed the nphdestruct branch from 6d61c3c to a030c3e Compare October 16, 2017 06:24

Replace NotePlayHandle::done() to the actual destructor

2d07efd

PhysSong force-pushed the nphdestruct branch from a030c3e to 2d07efd Compare October 16, 2017 06:26

lukas-w approved these changes Oct 16, 2017

View reviewed changes

PhysSong changed the title ~~Fix missing destructor call in NotePlayHandle~~ Fix destructor call in NotePlayHandleManager Oct 16, 2017

lukas-w merged commit 2930ef6 into LMMS:stable-1.2 Oct 16, 2017

PhysSong deleted the nphdestruct branch October 16, 2017 07:28

lukas-w added a commit that referenced this pull request Oct 17, 2017

PlayHandle: Zero out buffer before processing

6fc4577

Fixes buffer noises when instruments don't write the whole buffer, such as bitinvader. Related: * #3884 (comment): #3884 (comment) * #3883 # #3383

PhysSong mentioned this pull request Nov 24, 2017

Qt Assert failure: sendEvent across threads #4009

Closed

sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022

Merge pull request LMMS#3884 from PhysSong/nphdestruct

9bf3aec

Fix missing destructor call in NotePlayHandle

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix destructor call in NotePlayHandleManager #3884

Fix destructor call in NotePlayHandleManager #3884

PhysSong commented Oct 15, 2017

zonkmachine commented Oct 15, 2017

lukas-w Oct 15, 2017

PhysSong Oct 15, 2017

PhysSong Oct 15, 2017

lukas-w Oct 16, 2017

PhysSong Oct 16, 2017 •

edited

Loading

lukas-w Oct 16, 2017

PhysSong commented Oct 16, 2017

PhysSong commented Oct 16, 2017

zonkmachine commented Oct 17, 2017

lukas-w commented Oct 17, 2017 •

edited

Loading

zonkmachine commented Oct 17, 2017

lukas-w commented Oct 17, 2017

lukas-w commented Oct 17, 2017

zonkmachine commented Oct 17, 2017

PhysSong commented Oct 17, 2017

zonkmachine commented Oct 17, 2017

lukas-w commented Oct 18, 2017

zonkmachine commented Oct 18, 2017

Fix destructor call in NotePlayHandleManager #3884

Fix destructor call in NotePlayHandleManager #3884

Conversation

PhysSong commented Oct 15, 2017

zonkmachine commented Oct 15, 2017

lukas-w Oct 15, 2017

Choose a reason for hiding this comment

PhysSong Oct 15, 2017

Choose a reason for hiding this comment

PhysSong Oct 15, 2017

Choose a reason for hiding this comment

lukas-w Oct 16, 2017

Choose a reason for hiding this comment

PhysSong Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

lukas-w Oct 16, 2017

Choose a reason for hiding this comment

PhysSong commented Oct 16, 2017

PhysSong commented Oct 16, 2017

zonkmachine commented Oct 17, 2017

lukas-w commented Oct 17, 2017 • edited Loading

zonkmachine commented Oct 17, 2017

lukas-w commented Oct 17, 2017

lukas-w commented Oct 17, 2017

zonkmachine commented Oct 17, 2017

PhysSong commented Oct 17, 2017

zonkmachine commented Oct 17, 2017

lukas-w commented Oct 18, 2017

zonkmachine commented Oct 18, 2017

PhysSong Oct 16, 2017 •

edited

Loading

lukas-w commented Oct 17, 2017 •

edited

Loading